-
Notifications
You must be signed in to change notification settings - Fork 421
Remove Send + Sync bounds when no-std
#4184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4184 +/- ##
==========================================
- Coverage 88.86% 88.86% -0.01%
==========================================
Files 180 180
Lines 137869 137863 -6
Branches 137869 137863 -6
==========================================
- Hits 122520 122511 -9
- Misses 12539 12546 +7
+ Partials 2810 2806 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is broken, but I just confirmed that the current state works with LDK Node.
| &'a (dyn UtxoLookup + Send + Sync), | ||
| L, | ||
| > where | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change made rustfmt unhappy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, as I'm using rustfmt as well. Probably two different versions. Pushed a fix :)
valentinewallace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like CI is still sad :/
|
Sadly needs rebase now, but should be pretty trivial, also indeed the test kvstore utils need updating to the new API. I'd like to include this in 0.2, but we're rapidly running out of time for that. |
|
Would you mind rebasing on current git, rather than merging? We require each PR to be a freestanding list of commits, with no merges in them. |
2c534bf to
b165838
Compare
|
Feel free to squash the last two fixup commits into the second commit - we try to require that each commit individually builds and passes tests to ensure |
b165838 to
8e4b8e4
Compare
|
I hope everything looks good now! Sorry for the extra work earlier, and thanks for your guidance! :) |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! In the future it would be nice to get more context in the commit message but this time we'll let it slide :). Gonna go ahead and land this since @tnull said it doesn't break ldk-node and we want to get this in.
|
Backported in #4185 |
Summary
This PR addresses #4176
Details
SendandSyncbounds in thelightning-background-processorcrate and the asyncKVStoretrait, which were causing compilation issues when building without thestdfeature.no_stdenvironments while preserving async functionality.Rationale
These bounds were preventing the code from compiling in async, no-
stdconfigurations. By relaxing them, the async background processor and async persistence can now be used in embedded or WASM environments without requiringSend/Sync.